overlord,usersession: initial notifications of pending refreshes #9446

zyga · 2020-09-30T18:51:46Z

They say a picture is worth a thousand words and today I agree.

The new API allows snapd to notify userd, in each session, about a pending snap refresh. The notification carries the snap name and the amount of time remaining, before a forced refresh occurs. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

When refresh-app-awareness is enabled, snapd will not only issue snap warnings, but more importantly, send notifications to all the user sessions, carrying localized information about an upcoming snap refresh. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession/agent/rest_api.go

zyga · 2020-10-01T07:32:08Z

usersession/agent/rest_api.go

+
+	// TODO: silently ignore error returned when the notification server does not exist.
+	// TODO: track returned notification ID and respond to actions, if supported.
+	if _, err := notifySrv.SendNotification(msg); err != nil {


We could also track the returned notification ID for as long as userd is alive, and use it to update existing notification, if one exists. This would ensure that the roster of persistent notifications only shows the current, up-to-date entry for each snap, not a separate entry per notification.

This is one of the nicer parts of the the new GNOME notification spec: it has client provided notification IDs, so we don't need to track server IDs to update/replace/withdraw old notifications.

overlord/snapstate/autorefresh.go

usersession/agent/rest_api.go

bboozzoo · 2020-10-01T09:37:03Z

usersession/agent/rest_api.go

+	// XXX: how are instances supported in the snap store, are they?
+
+	// TODO: silently ignore error returned when the notification server does not exist.
+	// TODO: track returned notification ID and respond to actions, if supported.


Are we planning to do actual interaction here?

Given that this was literally designed last week I couldn't say.

If we want to have interactivity for FDO notifications, the idle tracking code will need updating to keep the session agent alive until the notification is dismissed. That's more of a TODO item than something that needs to be done in this PR though.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

I think this doesn't work the way the spec claims it to work. The notification sticks around until dismissed. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Thanks to Maciej for spotting that this runs with the state lock. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

merlijn-sebrechts · 2020-10-04T14:29:33Z

I'm not sure if the wording of the warning explains the risks clearly. I would interpret "disruptions" as 'the application will suddenly close". Is that the correct and only risk of missing the deadline?

I though refreshes would simply happen in the background. I thought that the application would keep running but some state and configuration might be lost the next time the application starts. Maybe something like "Close the app to avoid disruptions and data loss." is better?

zyga · 2020-10-05T07:21:03Z

I'm not sure if the wording of the warning explains the risks clearly. I would interpret "disruptions" as 'the application will suddenly close". Is that the correct and only risk of missing the deadline?

The application will not suddenly close, at least not in general. Some applications do observe issues related to the update of the apparmor profile.

I though refreshes would simply happen in the background. I thought that the application would keep running but some state and configuration might be lost the next time the application starts. Maybe something like "Close the app to avoid disruptions and data loss." is better?

Refreshes do happen in the background. This message informs the user that a refresh is possible but was postponed.

Still, thank you for the feedback. I'm sure the text will still change before this is released to the public by default.

anonymouse64

the code direction looks good to me, but I agree we need some more basic unit tests of this before we can merge it I think

anonymouse64 · 2020-10-06T04:02:59Z

overlord/snapstate/autorefresh.go

+			path := app.DesktopFile()
+			if _, err := os.Stat(path); err == nil {
+				refreshInfo.BusyAppName = appName
+				refreshInfo.BusyAppDesktopEntry = strings.SplitN(filepath.Base(path), ".", 2)[0]


is this robust enough because DesktopFile will always end in ".desktop" and we don't allow "." in the names of desktop files?

The desktop file is generated by us so yes. I think this is sensible.

This allows BusySnapError to be able to produce, by itself, PendingSnapRefreshInfo. I briefly experimented with a version that needs an externally-provided snap.Info, but it was more problematic as it could be called with the wrong info. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

This arrangement is not perfect, as those span packages (the return type is from a different package) but this feels more natural than the previous choice. While cleaning up this code, add missing tests for this logic. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

This will aid in testing and makes the code invoking it more readable, as the function is now not defined inline. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

In addition, mock the real interaction, even though they were harmless no-ops due to the lack of mocked socket. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

Remove relevant TODO Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

…ending-refresh

zyga · 2020-10-12T19:06:27Z

I've added unit tests for all the interactions now.

…ending-refresh

mvo5

Thanks for working on this, looks very nice. I have a bunch of comments/nitpicks but fine to address in followups I think. Giving people this better user experience seems well worth it!

mvo5 · 2020-10-21T07:24:17Z

overlord/snapstate/autorefresh.go

+				refreshInfo.TimeRemaining = (maxInhibition - now.Sub(*snapst.RefreshInhibitedTime)).Truncate(time.Second)
+				// Send the notification asynchronously to avoid holding the state lock.
+				asyncPendingRefreshNotification(context.TODO(), client, refreshInfo)
+				// XXX: remove the warning or send it only if no notification was delivered?


Yes, I think we should remove the warning. Fwiw, I think the warning here is really a bit of a misfeature because we cannot make it go away once the app was refreshed. But it's fine to keep the XXX and do that in a followup. Especially this first warning that just warns for the first time seems a bit unneeded.

I agree but I'd like to remove them in a follow up.

overlord/snapstate/autorefresh_test.go

mvo5 · 2020-10-21T07:30:21Z

overlord/snapstate/autorefresh_test.go

+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+		notificationSent = true
+		c.Check(refreshInfo.InstanceName, Equals, "pkg")
+		c.Check(refreshInfo.TimeRemaining, Equals, time.Hour*14*24/2-time.Second)


A comment here might be nice, something like: // just below the 14d window that triggers a stricter warning

I actually explained the XXX aspect of this test. Please see inline.

so this test is assuming that the test itself will always take less than one second? That seems ... brittle ... given our past experiences with running unit tests on arm* LP builders

mvo5 · 2020-10-21T07:34:43Z

overlord/snapstate/autorefresh.go

 			return err
 		}
 		if _, ok := err.(*BusySnapError); ok {
+			asyncPendingRefreshNotification(context.TODO(), client, refreshInfo)


This is slightly asymmetric, yes? I mean, we generate a warning here, i.e. the "policy" how strongly we warn is part of "autorefresh.go" here. But for the notifications we just give the "refreshInfo" and leave the "policy" about how strongly it warns to the "client.PendingRefreshNotification".

You are right but I think this resolves itself when the warnings go away.

Warnings are removed in a follow-up

mvo5 · 2020-10-21T07:50:13Z

overlord/snapstate/autorefresh.go

@@ -557,9 +583,16 @@ func inhibitRefresh(st *state.State, snapst *SnapState, info *snap.Info, checker
 		if now.Sub(*snapst.RefreshInhibitedTime) < maxInhibition {
 			// If we are still in the allowed window then just return
 			// the error but don't change the snap state again.
+			refreshInfo.TimeRemaining = (maxInhibition - now.Sub(*snapst.RefreshInhibitedTime)).Truncate(time.Second)
+			asyncPendingRefreshNotification(context.TODO(), client, refreshInfo)


(nitpick) we have the notification sending code in very small variations three times here, I wonder if this could be somehow simplified but definitely more a followup.

I will do this with the removal of the warning, in a follow-up.

Done (in a follow-up that needs this to land first)

overlord/snapstate/refresh.go

mvo5 · 2020-10-21T08:23:16Z

usersession/agent/rest_api.go

+	var body string
+	var icon string
+	var hints []notification.Hint
+	if daysLeft := int(refreshInfo.TimeRemaining.Truncate(time.Hour).Hours() / 24); daysLeft > 0 {


I would suggest forming the parenthesised part of the message independently, then construct the body using Close the app to avoid disruptions (%s) as the format string. This would remove the need for translators to repeat the same translation 6+ times (depending on how many plural forms the language has).

We've got similar formatting code in timeutil/human.go for formatting times rather than durations, so perhaps that part of the formatting could be moved there (either in this PR or a follow-up).

usersession/client/client.go

usersession/agent/rest_api.go

zyga · 2020-10-22T10:58:53Z

I'm iterating on this now, mainly adding tests but not just that. It should be ready today.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Instead of opening the snap-store page, it's better to ask the application to close. This will be a follow-up, at least for X11 systems. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Are we green? Super green! Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This way we detect malformed number of attempts Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

jhenstridge

I've only considered the session agent/notifications sections of the PR in this review.

usersession/agent/rest_api.go

jhenstridge · 2020-10-23T09:37:18Z

usersession/agent/rest_api.go

+	var body string
+	var icon string
+	var hints []notification.Hint
+	if daysLeft := int(refreshInfo.TimeRemaining.Truncate(time.Hour).Hours() / 24); daysLeft > 0 {


I would suggest forming the parenthesised part of the message independently, then construct the body using Close the app to avoid disruptions (%s) as the format string. This would remove the need for translators to repeat the same translation 6+ times (depending on how many plural forms the language has).

We've got similar formatting code in timeutil/human.go for formatting times rather than durations, so perhaps that part of the formatting could be moved there (either in this PR or a follow-up).

usersession/agent/rest_api.go

jhenstridge · 2020-10-23T10:11:53Z

usersession/agent/rest_api.go

+	// XXX: how are instances supported in the snap store, are they?
+
+	// TODO: silently ignore error returned when the notification server does not exist.
+	// TODO: track returned notification ID and respond to actions, if supported.


If we want to have interactivity for FDO notifications, the idle tracking code will need updating to keep the session agent alive until the notification is dismissed. That's more of a TODO item than something that needs to be done in this PR though.

jhenstridge · 2020-10-23T10:14:32Z

usersession/agent/rest_api.go

+
+	// TODO: silently ignore error returned when the notification server does not exist.
+	// TODO: track returned notification ID and respond to actions, if supported.
+	if _, err := notifySrv.SendNotification(msg); err != nil {


This is one of the nicer parts of the the new GNOME notification spec: it has client provided notification IDs, so we don't need to track server IDs to update/replace/withdraw old notifications.

jhenstridge · 2020-10-23T10:17:58Z

usersession/agent/rest_api.go

+	notifySrv := notification.New(conn)
+
+	// TODO: this message needs to be crafted better as it's the only thing guaranteed to be delivered.
+	summary := fmt.Sprintf(i18n.G("Pending update of %q snap"), refreshInfo.InstanceName)


Given we have the desktop entry of the busy application, perhaps it would be worth including the name from that too? That potentially gives us a localised name. As it is unverified information from the client though, we probably still want to include the snape name.

I've experimented with some of that from the side of snapd, where we know the app name that is busy. It tends to create repetitive patterns (chrome chrome ...) and I didn't like the result. Desktop file names are sometimes equally weird so I'd postpone that until we can give it a try on a larger sample size.

The connection is opened once and shared within the process. It should not be closed here. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

This way the session agent is not pretending to be the busy snap application but correctly represents itself. The icon is still loaded from the desktop file, if we can connect the dots between the application and the desktop file, so that the icon is familiar to the user. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

anonymouse64

LGTM, I am slightly worried about the brittleness of one test, but can certainly be cleaned up in a followup.

Thanks for all the work here!

anonymouse64 · 2020-10-23T15:38:19Z

overlord/snapstate/autorefresh_test.go

+	restore := snapstate.MockAsyncPendingRefreshNotification(func(ctx context.Context, client *userclient.Client, refreshInfo *userclient.PendingSnapRefreshInfo) {
+		notificationSent = true
+		c.Check(refreshInfo.InstanceName, Equals, "pkg")
+		c.Check(refreshInfo.TimeRemaining, Equals, time.Hour*14*24/2-time.Second)


so this test is assuming that the test itself will always take less than one second? That seems ... brittle ... given our past experiences with running unit tests on arm* LP builders

anonymouse64 · 2020-10-23T21:12:36Z

usersession/agent/rest_api_test.go

+	})
+}
+
+func (s *restSuite) TestPostPendingRefreshNotificationNoNotificatinServer(c *C) {


Suggested change

func (s *restSuite) TestPostPendingRefreshNotificationNoNotificatinServer(c *C) {

func (s *restSuite) TestPostPendingRefreshNotificationNoNotificationServer(c *C) {

I'll send a follow-up for this.

Sent as #9552

jhenstridge

This looks good. I think eventually it would be good to convert over to running against a real (private) session bus. That can come later though.

As with last review, this approval is only taking the session agent and notification portions of the PR into account.

zyga added this to In progress in Refresh App Awareness via automation Sep 30, 2020

zyga force-pushed the feature/notify-on-pending-refresh branch from b713034 to 489ea95 Compare October 1, 2020 06:31

zyga added 2 commits October 1, 2020 08:54

zyga force-pushed the feature/notify-on-pending-refresh branch from 489ea95 to 0e7e301 Compare October 1, 2020 06:55

bboozzoo self-requested a review October 1, 2020 07:00

zyga commented Oct 1, 2020

View reviewed changes

usersession/agent/rest_api.go Outdated Show resolved Hide resolved

usersession/agent/rest_api.go Outdated Show resolved Hide resolved

zyga commented Oct 1, 2020

View reviewed changes

bboozzoo reviewed Oct 1, 2020

View reviewed changes

zyga added 4 commits October 1, 2020 13:26

usersession: add missing i18n.G of notification text

f0b40bc

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: remove debug printf

e9eb7f1

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: remove ineffective ExpireTimeout

6066d68

I think this doesn't work the way the spec claims it to work. The notification sticks around until dismissed. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/snapstate: send notifications asynchronously

1be2b1a

Thanks to Maciej for spotting that this runs with the state lock. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

anonymouse64 self-requested a review October 5, 2020 12:46

anonymouse64 reviewed Oct 6, 2020

View reviewed changes

zyga added 7 commits October 12, 2020 19:21

overlord: add TODO about missing tests

db810ed

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

overlord: make asyncPendingRefreshNotification mockable

99295dd

This will aid in testing and makes the code invoking it more readable, as the function is now not defined inline. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

overlord: test dispatch of desktop notifications

2a36c49

In addition, mock the real interaction, even though they were harmless no-ops due to the lack of mocked socket. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

overlord: test non-overdue refresh notification

670b01a

Remove relevant TODO Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

Merge remote-tracking branch 'origin/master' into feature/notify-on-p…

b6be078

…ending-refresh

zyga added 2 commits October 13, 2020 23:35

Merge remote-tracking branch 'origin/master' into feature/notify-on-p…

24dbf7c

…ending-refresh

Merge remote-tracking branch 'origin/master' into feature/notify-on-p…

bf12216

…ending-refresh

mvo5 approved these changes Oct 21, 2020

View reviewed changes

zyga added 10 commits October 22, 2020 14:43

usersession: use refreshInfo to refer to PendingSnapRefreshInfo

9981312

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: add smoke test for PendingRefreshNotification client call

59d3475

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: use SnapDesktopFilesDir over custom constant

c9e348d

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: drop TODO about snap-store

12434b8

Instead of opening the snap-store page, it's better to ask the application to close. This will be a follow-up, at least for X11 systems. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: explain TODO better

b2fdb1f

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: add tests for /v1/notifications/pending-refresh

48c80b4

Are we green? Super green! Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/snapstate: use test counter, not flag

17eb9e7

This way we detect malformed number of attempts Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/snapstate: explain some test measurements

f680c1e

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

overlord/snapstate: use osutil.FileExists

31dec23

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: tweak variable declaration

c487d86

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga requested a review from jhenstridge October 22, 2020 15:33

anonymouse64 self-requested a review October 22, 2020 17:45

jhenstridge requested changes Oct 23, 2020

View reviewed changes

zyga added 4 commits October 23, 2020 13:50

usersession: do not close shared D-Bus connection

3d43e83

The connection is opened once and shared within the process. It should not be closed here. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: re-factor i18n handling to avoid repetition

268557e

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

usersession: add TODO for sharing DBus connection

20f559a

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

zyga requested a review from jhenstridge October 23, 2020 12:04

anonymouse64 approved these changes Oct 23, 2020

View reviewed changes

jhenstridge approved these changes Oct 26, 2020

View reviewed changes

mvo5 merged commit e81a5e7 into snapcore:master Oct 26, 2020

Refresh App Awareness automation moved this from In progress to Done Oct 26, 2020

zyga deleted the feature/notify-on-pending-refresh branch October 26, 2020 10:10

	func (s restSuite) TestPostPendingRefreshNotificationNoNotificatinServer(c C) {
	func (s restSuite) TestPostPendingRefreshNotificationNoNotificationServer(c C) {

overlord,usersession: initial notifications of pending refreshes #9446

overlord,usersession: initial notifications of pending refreshes #9446

Conversation

zyga commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlijn-sebrechts commented Oct 4, 2020 • edited

zyga commented Oct 5, 2020 • edited

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga commented Oct 12, 2020

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga commented Oct 22, 2020

jhenstridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhenstridge left a comment

Choose a reason for hiding this comment

merlijn-sebrechts commented Oct 4, 2020 •

edited

zyga commented Oct 5, 2020 •

edited